Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Collapsed subprocess paste & undo bugfix #2275

Draft
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

jarekdanielak
Copy link
Contributor

@jarekdanielak jarekdanielak commented Jan 17, 2025

Problem

If you copy & paste a collapsed subprocess with some elements inside and try to undo this action, editor crashes.

Root cause

When a collapsed subprocess is pasted, elements.create command is triggered and triggers following actions:

  1. shape.create for a new subprocess element.
  2. elements.move for subprocess children (triggered post-shape.create by `SubProcessPlaneBehavior)
  3. shape.create for each child.

When this action is being undone, subprocess children are first removed (revert shape.create) and then are attempted to be moved back to their original parent/position (revert elements.move) which doesn't make sense as those elements are already removed.

Proposed solution

I believe the correct implementation of subprocess plane behavior should move the elements onto the new plane after they are all created, so on postExecuted of elements.create instead of shape.create.

This pull requests does it and therefore solve the paste & undo bug.

New problem

Unfortunately, the subprocess element in the context of elements.create handler doesn't have all its children. It's missing labels and text annotations. Those elements are still copied, but remain hidden on the root plane.

Additional context

Initial discussion and debugging session: bpmn-io/diagram-js#957 (comment).

Closes #2269

Checklist

To ensure you provided everything we need to look at your PR:

  • Brief textual description of the changes present
  • Visual demo attached
  • Steps to try out present, i.e. using the @bpmn-io/sr tool
  • Related issue linked via Closes {LINK_TO_ISSUE} or Related to {LINK_TO_ISSUE}

@jarekdanielak jarekdanielak marked this pull request as draft January 17, 2025 20:03
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending in progress Currently worked on and removed needs review Review pending labels Jan 17, 2025
@jarekdanielak
Copy link
Contributor Author

I've spent some more time debugging this and updated the top comment with my latest conclusions.

The proposed solution of moving children elements after elements.create looks like the most sane way of implementing this behavior, but I still have some questions:

  • In the current implementation of SubProcessPlaneBehavior, why is shape.create executed for each pasted subprocess child, even though they seem to be already available in the post-shape.create of the subprocess element?
  • Why are all the children available in the context of shape.create, but not in the context of elements.create?

I'd like to come back to this in a paired debugging session. cc. @barmac @nikku

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in progress Currently worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Copy + Paste and then Undo Collapsed Sub process throws JS Errors
1 participant